Fix build and tests with pari 2.17 #38749

antonio-rojas · 2024-10-01T17:12:20Z

NEXT_PRIME_VIADIFF is removed in 2.17, port pari_prime_range to pari_PRIMES instead

Needs sagemath/cypari2#165 applied to cypari

antonio-rojas · 2024-10-01T17:14:07Z

Needs work because this is not compatible with 2.15. So either it needs to be merged with the pari upgrade or be made to work with older pari too (no idea how).

Running tests now.

antonio-rojas · 2024-10-03T07:43:45Z

Many test failures.

jhpalmieri · 2024-10-05T20:58:48Z

If the changes here eventually allow us to use a system Pari 2.17, then we should undo the change in #38772.

antonio-rojas · 2024-10-06T11:27:40Z

With this MR and the cypari fixes sagemath/cypari2#165 and sagemath/cypari2#166 all tests are passing with pari 2.17.

The changes are not compatible with 2.15 though, making them compatible requires more work. Also, some pari opeations (such as the number field prime ideals above a given prime) give random output with 2.17, which makes it harder to test. To solve both issues (and make tests more future proof), we should gradually move away from testing the exact output to just testing that the output is correct.

src/sage/schemes/elliptic_curves/ell_point.py

NEXT_PRIME_VIADIFF is removed in 2.17, port pari_prime_range to pari_PRIMES instead

Co-authored-by: Gonzalo Tornaría <[email protected]>

tornaria · 2024-11-04T00:32:02Z

There seems to be a major issue with pari 2.17: the computation of the units of a bnf is not deterministic:

? setrand(1) ; bnfinit(x^2-65).fu
%1 = [Mod(x - 8, x^2 - 65)]
? setrand(3) ; bnfinit(x^2-65).fu
%2 = [Mod(-x + 8, x^2 - 65)]

And also (related?) generators of ideals:

$ sage -c 'pari.setrand(1) ; K.<a> = QuadraticField(-6) ; L.<b> = K.extension(polygen(K)^4 + a) ; print(L.ideal(b).relative_norm())'
Fractional ideal (-a)
$ sage -c 'pari.setrand(3) ; K.<a> = QuadraticField(-6) ; L.<b> = K.extension(polygen(K)^4 + a) ; print(L.ideal(b).relative_norm())'
Fractional ideal (a)

and factorization are non-deterministic as well:

$ sage -c 'pari.setrand(1) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(omega) * (-3*omega - 2)
$ sage -c 'pari.setrand(2) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(omega + 1) * (-2*omega + 1)
$ sage -c 'pari.setrand(4) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
omega + 3
$ sage -c 'pari.setrand(7) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(-omega) * (3*omega + 2)
$ sage -c 'pari.setrand(24) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(-1) * (-omega - 3)
$ sage -c 'pari.setrand(44) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(-omega - 1) * (2*omega - 1)

This is making tests a bit unstable, because running in --long or not reaches the examples with different pari random state...

We need to remove this function from cypari2, because pari 2.17 has changed precision from words to bits which would cause confusion. Besides, the current usage is incorrect, since `log_pari.precision()` will give the precision in *decimal digits* not in words. See: sagemath#38749 ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. URL: sagemath#38908 Reported by: Gonzalo Tornaría Reviewer(s): Vincent Delecroix

antonio-rojas · 2024-11-06T07:40:15Z

There seems to be a major issue with pari 2.17: the computation of the units of a bnf is not deterministic:

Yes, see #38749 (comment). I have only tested with --long and still find failing tests from time to time due to this. The reasonable solution is to make all tests independent of the exact output, but that is a lot of work.

antonio-rojas · 2024-11-06T07:44:06Z

I am also seeing one test failure with a specific random seed:

sage -t --long --warn-long 30.0 --random-seed=164564093241797715927265696104150551458 /usr/lib/python3.12/site-packages/sage/rings/integer.pyx
**********************************************************************
File "/usr/lib/python3.12/site-packages/sage/rings/integer.pyx", line 3074, in sage.rings.integer.Integer.divisors
Failed example:
    for i in range(20):           # long time                             # needs sage.libs.pari
        try:
            alarm(RDF.random_element(1e-3, 0.5))
            _ = n.divisors()
            cancel_alarm()  # we never get here
        except AlarmInterrupt:
            pass
Exception raised:
    Traceback (most recent call last):
      File "<doctest sage.rings.integer.Integer.divisors[20]>", line 4, in <module>
        _ = n.divisors()
            ^^^^^^^^^^^^
      File "sage/rings/integer.pyx", line 3160, in sage.rings.integer.Integer.divisors (build/cythonized/sage/rings/integer.c:31916)
        for p, e in f:
      File "/usr/lib/python3.12/site-packages/sage/structure/factorization.py", line 327, in __getitem__
        def __getitem__(self, i):
        
      File "src/cysignals/signals.pyx", line 341, in cysignals.signals.python_check_interrupt
    cysignals.signals.AlarmInterrupt

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 1136, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.integer.Integer.divisors[20]>", line 6, in <module>
        except AlarmInterrupt:
               ^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sage/doctest/util.py", line 497, in __getitem__
        def __getitem__(self, name):
        
      File "src/cysignals/signals.pyx", line 341, in cysignals.signals.python_check_interrupt
    cysignals.signals.AlarmInterrupt
**********************************************************************

This is not reproducible with pari 2.15, but I don't know if it's because of some pari change or because the pari upgrade influences the choice of random timeouts. The obvious fix (adding from cysignals import AlarmInterrupt as in line 7119) doesn't help.

tornaria · 2024-11-06T14:28:49Z

@antonio-rojas I can reproduce it deterministically with pari 2.15 and 2.17 with this change:

--- a/src/sage/rings/integer.pyx	2024-08-23 10:46:19.000000000 -0300
+++ b/src/sage/rings/integer.pyx	2024-11-06 11:23:36.666702122 -0300
@@ -3075,9 +3075,9 @@
         so a memory leak will not go unnoticed)::
 
             sage: n = prod(primes_first_n(25))                                          # needs sage.libs.pari
-            sage: for i in range(20):           # long time                             # needs sage.libs.pari
+            sage: for t in [0.5, 0.003]:
             ....:     try:
-            ....:         alarm(RDF.random_element(1e-3, 0.5))
+            ....:         alarm(t)
             ....:         _ = n.divisors()
             ....:         cancel_alarm()  # we never get here
             ....:     except AlarmInterrupt:

I couldn't reproduce this outside doctesting this file.

EDIT: the point is that with this change, the failure happens every time, regardless of seed, and regardless of --long. The randomness is just hitting a sequence of timeouts where this triggers (i.e. a very small timeout after a largish timeout as I did up there).

tornaria · 2024-11-06T14:34:09Z

There seems to be a major issue with pari 2.17: the computation of the units of a bnf is not deterministic:

Yes, see #38749 (comment). I have only tested with --long and still find failing tests from time to time due to this. The reasonable solution is to make all tests independent of the exact output, but that is a lot of work.

Sure, but there is still something weird with this. Some of the E.isogeny_class() computations take a very long time (randomly) and I'm wondering if this is the cause.

I found other issues with reduced_basis() which I tracked back to the new LLL being quite unstable (numerically) and in that case I solved by reverting to the old LLL. Now I am wondering if all this unstability in units, factoring, generators, may be due to the same LLL unstability.

In principle, this new FLATTER version of LLL is better (faster) but...

tornaria · 2024-11-06T14:40:35Z

BTW: I'm preparing a patch for 32-bit. Besides all the unstability, pari 2.17 changed the default real precision to be 128 bits for both 64-bit and 32-bit arches (previously it was 128 bits = 38 digits for 64-bit arch and 96 bit = 28 digits for 32-bit arch).

We need to remove this function from cypari2, because pari 2.17 has changed precision from words to bits which would cause confusion. Besides, the current usage is incorrect, since `log_pari.precision()` will give the precision in *decimal digits* not in words. See: sagemath#38749 ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. URL: sagemath#38908 Reported by: Gonzalo Tornaría Reviewer(s): Vincent Delecroix

tornaria · 2024-11-08T02:29:39Z

More updates in antonio-rojas#4

We need to remove this function from cypari2, because pari 2.17 has changed precision from words to bits which would cause confusion. Besides, the current usage is incorrect, since `log_pari.precision()` will give the precision in *decimal digits* not in words. See: sagemath#38749 ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. URL: sagemath#38908 Reported by: Gonzalo Tornaría Reviewer(s): Vincent Delecroix

github-actions bot added the s: needs review label Oct 1, 2024

antonio-rojas added s: needs work and removed s: needs review labels Oct 1, 2024

github-actions bot added s: needs review and removed s: needs work labels Oct 2, 2024

antonio-rojas changed the title ~~Fix build with pari 2.17~~ [WIP] Fix build with pari 2.17 Oct 3, 2024

antonio-rojas changed the title ~~[WIP] Fix build with pari 2.17~~ [WIP] Fix build and tests with pari 2.17 Oct 3, 2024

jhpalmieri mentioned this pull request Oct 5, 2024

external Pari 2.17 does not work (yet) #38769

Closed

2 tasks

antonio-rojas force-pushed the pari-2.17 branch from a6acbfb to f619b6f Compare October 5, 2024 20:01

antonio-rojas changed the title ~~[WIP] Fix build and tests with pari 2.17~~ Fix build and tests with pari 2.17 Oct 6, 2024

antonio-rojas force-pushed the pari-2.17 branch from 257246c to b3c57e1 Compare October 12, 2024 09:59

antonio-rojas added s: needs work and removed s: needs review labels Oct 12, 2024

github-actions bot added s: needs review and removed s: needs work labels Oct 12, 2024

antonio-rojas added s: needs work and removed s: needs review labels Oct 16, 2024

collares mentioned this pull request Oct 22, 2024

pari: 2.15.5 -> 2.17.0 NixOS/nixpkgs#348442

Open

github-actions bot added s: needs review and removed s: needs work labels Oct 27, 2024

antonio-rojas added s: needs work and removed s: needs review labels Oct 27, 2024

tornaria reviewed Oct 30, 2024

View reviewed changes

src/sage/schemes/elliptic_curves/ell_point.py Outdated Show resolved Hide resolved

tornaria reviewed Oct 30, 2024

View reviewed changes

src/sage/schemes/elliptic_curves/ell_point.py Outdated Show resolved Hide resolved

github-actions bot added s: needs review and removed s: needs work labels Oct 30, 2024

antonio-rojas and others added 8 commits October 30, 2024 22:13

Fix build with pari 2.17

f59ea66

NEXT_PRIME_VIADIFF is removed in 2.17, port pari_prime_range to pari_PRIMES instead

Adapt new_gen_from_real_mpfr_element to pari 2.17 changes in precision

dee7334

Update tests output for pari 2.17

34af90b

Account for random output in some tests

48f18f1

Rename prec_words_to_bits to prec_pari_to_bits

ff1b011

Account for more pari 2.17 random output in some tests

14dd562

Replace prec_pari_to_bits usage

23ff7d6

Co-authored-by: Gonzalo Tornaría <[email protected]>

Replace prec_pari_to_bits usage

fbb32fd

Co-authored-by: Gonzalo Tornaría <[email protected]>

tornaria mentioned this pull request Nov 2, 2024

Don't (mis)use prec_words_to_bits() #38908

Merged

3 tasks

tornaria mentioned this pull request Nov 6, 2024

Fix doctests for pari 2.17 on 32-bit antonio-rojas/sage#3

Merged

antonio-rojas force-pushed the pari-2.17 branch from 6d7c793 to fbb32fd Compare November 16, 2024 17:07

Merge remote-tracking branch 'upstream/develop' into pari-2.17

91db307

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build and tests with pari 2.17 #38749

Fix build and tests with pari 2.17 #38749

antonio-rojas commented Oct 1, 2024

antonio-rojas commented Oct 1, 2024

antonio-rojas commented Oct 3, 2024

jhpalmieri commented Oct 5, 2024

antonio-rojas commented Oct 6, 2024 •

edited

Loading

tornaria commented Nov 4, 2024

antonio-rojas commented Nov 6, 2024

antonio-rojas commented Nov 6, 2024

tornaria commented Nov 6, 2024 •

edited

Loading

tornaria commented Nov 6, 2024

tornaria commented Nov 6, 2024

tornaria commented Nov 8, 2024

Fix build and tests with pari 2.17 #38749

Are you sure you want to change the base?

Fix build and tests with pari 2.17 #38749

Conversation

antonio-rojas commented Oct 1, 2024

antonio-rojas commented Oct 1, 2024

antonio-rojas commented Oct 3, 2024

jhpalmieri commented Oct 5, 2024

antonio-rojas commented Oct 6, 2024 • edited Loading

tornaria commented Nov 4, 2024

antonio-rojas commented Nov 6, 2024

antonio-rojas commented Nov 6, 2024

tornaria commented Nov 6, 2024 • edited Loading

tornaria commented Nov 6, 2024

tornaria commented Nov 6, 2024

tornaria commented Nov 8, 2024

antonio-rojas commented Oct 6, 2024 •

edited

Loading

tornaria commented Nov 6, 2024 •

edited

Loading